Implementation of STLSubdomainGenerator#32764
Implementation of STLSubdomainGenerator#32764zachmprince wants to merge 6 commits intoidaholab:nextfrom
STLSubdomainGenerator#32764Conversation
|
Job Documentation, step Docs: sync website on d9de425 wanted to post the following: View the site here This comment will be updated on new commits. |
|
Job Coverage, step Generate coverage on af5b335 wanted to post the following: Framework coverage
Modules coverageCoverage did not change Full coverage reportsReports
This comment will be updated on new commits. |
||||||||||||||||||||||||||
dac195b to
623540e
Compare
|
Here's a Claude review Overall AssessmentThe implementation is well-structured and the core algorithms (Möller-Trumbore ray intersection, Bug: Negative
|
| Range | Meaning | Correct result |
|---|---|---|
t < 0 |
Intersection is behind the ray origin (−x direction) | Miss |
0 <= t <= surface_tolerance |
Intersection is near the surface of the query point | Ambiguous |
Currently both map to Ambiguous, which causes containsBySolidAngle() (an O(n) scan of all
triangles) to be invoked for every triangle that straddles the query point's x-coordinate but
whose actual plane hit is in the −x direction. In contains(), an Ambiguous result immediately
returns, abandoning the parity loop early, so these misfires produce a correct but very expensive
result.
The pre-filter tri.bbox.max()(0) < point(0) - _surface_tolerance does not exclude this case —
a triangle whose bounding box straddles the query point's x can still have its plane intersection
at a negative t.
Fix:
const Real t = inv_determinant * (edge2 * q);
if (t < 0.0)
return RayIntersection::Miss;
if (t <= _surface_tolerance)
return RayIntersection::Ambiguous;Minor: !probe Check Has Inverted Logic
File: framework/src/utils/STLManifold.C, parse()
probe.read(header.data(), header.size());
if (!probe && file_size >= header.size())
mooseError("Failed while reading STL file '", file_name, "'.");std::ifstream::read() sets failbit on any short read (including EOF), so !probe fires for
files smaller than 84 bytes regardless of the intent. The file_size >= header.size() guard
happens to work because file_size is captured before the read, but the logic is subtle.
Using gcount() makes the intent explicit:
if (probe.gcount() < static_cast<std::streamsize>(header.size()) && file_size >= header.size())
mooseError("Failed while reading STL file '", file_name, "'.");Minor: Binary Detection Can Misfire on an 84-Byte ASCII File
File: framework/src/utils/STLManifold.C, parse()
const auto expected_size = static_cast<std::size_t>(84ull + 50ull * triangle_count);
parse_as_binary = expected_size == file_size;If bytes 80–83 of the ASCII file are all zero (common when the solid name is short or absent),
triangle_count reads as 0 and expected_size becomes 84. An ASCII file that is exactly 84
bytes long is then parsed as binary and immediately fails with "does not contain any triangles."
Adding a zero-count guard eliminates the ambiguity:
parse_as_binary = triangle_count > 0 && expected_size == file_size;Design Observations
All MPI ranks read the STL file.
generate() runs on every rank and constructs STLManifold (which opens and reads the file) on
each. For thousands of ranks on large parallel systems this causes a file-system storm. A common
MOOSE pattern is to restrict file I/O to rank 0 and broadcast the parsed data, though the current
behavior is consistent with other mesh generators in the framework.
surface_tolerance is an absolute tolerance with a very small default.
The manifold validation uses surface_tolerance = 1e-10 for vertex quantization. If the STL file
exports vertices with floating-point noise at the ~1e-9 level (not uncommon for some CAD
exporters), coincident vertices will not be stitched and the non-watertight error will fire
unexpectedly. The documentation and parameter description should emphasize that this value must be
chosen relative to the actual geometry scale.
No warning when block_id already exists.
If the user specifies a block_id that is already assigned to elements outside the STL manifold,
those elements are silently left unchanged while elements inside the manifold are overwritten. This
is consistent with SubdomainBoundingBox and similar generators, but a mooseWarning when the
target block already has elements could prevent user confusion.
Summary
| Severity | File | Location | Issue |
|---|---|---|---|
| Bug | STLManifold.C |
rayIntersectsTriangle() |
Negative t returns Ambiguous instead of Miss, triggering spurious O(n) solid-angle fallback |
| Minor | STLManifold.C |
parse() |
!probe error check should use gcount() to be explicit about a short read |
| Minor | STLManifold.C |
parse() |
Binary detection misfires on an 84-byte ASCII file whose bytes 80–83 are zero |
| Design | STLSubdomainGenerator.C |
generate() |
All MPI ranks open and parse the STL file independently |
GiudGiud
left a comment
There was a problem hiding this comment.
Claude reviewed too fast for me
| @@ -0,0 +1,33 @@ | |||
| # STLSubdomainGenerator | |||
|
|
|||
| !syntax description /Mesh/STLSubdomainGenerator | |||
There was a problem hiding this comment.
ideally the domain definition would be with "surface meshes" or "CAD" or "Geometry" (englobing both CSG, CAD and meshes) not STL
we don't real want an OBJSubdomainGenerator or STEPSubdomainGenerator to come next
There was a problem hiding this comment.
and this only supports STLs that are manifolds
There was a problem hiding this comment.
This seems like a bigger design decision we should talk about.
There was a problem hiding this comment.
maybe the abstraction needs to be a "TriangulationManifold" instead of STL
it s still somewhat similar to a surface mesh though
then we can have a "read_from_STL" routine for now, and expand as needed for future formats
(for example build_from_mesh could be cool?)
There was a problem hiding this comment.
Yeah - "STL" both overpromises (STL meshes aren't even guaranteed to be watertight, much less manifolds) and underpromises (there are other good ways to get a watertight Tri manifold). I'm afraid I can't think of a much better name than "TriangulationManifold", which is flawed (very wordy, only obvious to people who've heard the topological definition of manifold) but which is probably good enough. Maybe "TriangulatedSurface" would be clearer to most users? Or "TriangulatedBoundary" might be good - I'd think they mean the same thing to most users but to me there's more connotation of "the surface can't be self-intersecting, and it has to be a manifold-without-boundary so it can itself be a complete boundary of a volume").
There was a problem hiding this comment.
We could probably just shorten "Triangulation" or "Triangulated" to "Tri". New suggestions: "TriSurfaceSubdomainGenerator" or "TriEnclosedSubdomainGenerator". My "Boundary" suggestion was a bad one, since in MOOSE (and libMesh, and among analysts in general) we liberally use "boundary" to mean only subsets of a full domain boundary.
There was a problem hiding this comment.
TriangulatedSurface
This would be OK to me too as long as all the routines that only support manifold get either:
- a boolean "is_manifold" (either in the class definition OR in the routine's parameters) and a debug check on "is actually manifold". error on is_manifold = false works for me in those routines
- a "AsManifold" or some other indication in the name of the routine. Probably ugly
| const RealVectorValue & translation); | ||
|
|
||
| /// Parse an ASCII STL stream. | ||
| void parseASCII(std::istream & input, |
There was a problem hiding this comment.
we have an ASCII reader in libmesh for stl.
We could survive with an extra copy temporarily if this is getting expanded into something else next year. We likely need to discuss what's acceptable
There was a problem hiding this comment.
I didn't even know libmesh had this, oops. Let me look into it.
There was a problem hiding this comment.
It supports binary STL now too. There's not much test coverage, though, just a few test meshes in CI and a few more manually tried in personal projects; let me know if you hit any bugs.
| closed STL manifold. The STL surface must be watertight and manifold. Element centroids that lie on | ||
| the STL surface within the configured tolerance are treated as inside. | ||
|
|
||
| This generator reads an STL file directly for containment tagging only. It does not make STL a |
There was a problem hiding this comment.
| This generator reads an STL file directly for containment tagging only. It does not make STL a | |
| This generator reads an STL file directly for assigning subdomains in another mesh. It does not make STL a |
| // A non-positive tolerance would make both quantization and surface classification ill-defined. | ||
| if (_surface_tolerance <= 0.0) | ||
| mooseError("surface_tolerance must be strictly positive."); |
| _bounding_box = std::make_unique<libMesh::BoundingBox>(_triangles.back().bbox); | ||
| else | ||
| // Update the global bounds incrementally as triangles are added. | ||
| _bounding_box = std::make_unique<libMesh::BoundingBox>( |
There was a problem hiding this comment.
we need to update the coordinates we dont need to re-allocate a BBOX
GiudGiud
left a comment
There was a problem hiding this comment.
clicked some resolve buttons
| points that lie on the STL surface within the configured tolerance are treated as inside. | ||
|
|
||
| This generator reads an STL file directly for assigning subdomains in another mesh. It does not make STL a | ||
| general-purpose input format, like those supported in [FileMeshGenerator](FileMeshGenerator.md). |
There was a problem hiding this comment.
| general-purpose input format, like those supported in [FileMeshGenerator](FileMeshGenerator.md). | |
| general-purpose mesh input format, as does the [FileMeshGenerator](FileMeshGenerator.md) for several other formats. |
…a STL file. A new utilility class---STLManifold---is implemented, which determines whether a point is enclosed in the STL geometry. A new mesh generator---STLSubdomainGenerator---is implemented, which assigns a specified subdomain based on whether the element centroid is within the manifold specified by the STL file. Refs idaholab#32763
- Fixed negative intersection check so that it is a miss instead of ambiguous - Using stream gcount for safer handling of failed file reads - Making the ascii vs. binary file detection robust for special cases where byte size is exactly 84 - Clarifying surface_tolerance logic in documentation - Throwing a mooseInfo when block_id already exists in the mesh
- Clarifying language in md about subdomain selection - Specifying that this it uses vertex_average, not centroid - Using mooseAssert's in STLManifold constructor - Avoid reallocating bounding box in STLManifold - Ensuring parallel-check uses consistent units for the tolerance
- Making statement about FileMeshGenerator less ambiguous - Remove usage of "manifold" as an adjective - Being more explicit in doxygen than "ray casting" - Setting only element data as unprepared
|
Job Precheck, step Clang format on cced4a0 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
… an STL file. Summary of changes: - STLManifold -> TriangleManifold - STLSubdomainGenerator -> ManifoldSubdomainGenerator - stl_subdomain_generator -> manifold_subdomain - STLManifoldTest -> TriangleManifoldTest Refs idaholab#32763
|
Job Test, step Results summary on d9de425 wanted to post the following: Framework test summaryCompared against 4e53b3b in job civet.inl.gov/job/3786076. Added tests
Modules test summaryCompared against 4e53b3b in job civet.inl.gov/job/3786076. No change |
| consistently oriented. Vertex-average points that lie on the surface within the configured tolerance | ||
| are treated as inside. | ||
|
|
||
| The manifold is supplied as a mesh via the MeshGenerator pipeline, not as a raw file path. To use an |
There was a problem hiding this comment.
| The manifold is supplied as a mesh via the MeshGenerator pipeline, not as a raw file path. To use an | |
| The manifold is supplied as a mesh via the MeshGenerator pipeline, not as a raw file path. For example, to use an |
There was a problem hiding this comment.
another example you could also mention is how to convert your average non-tri surface mesh to a tri surface mesh
| * | ||
| * The generator intentionally uses Elem::vertex_average() rather than the true geometric centroid | ||
| * to mirror the inexpensive point-sampling behavior of other subdomain tagging mesh generators in | ||
| * MOOSE. |
There was a problem hiding this comment.
| * | |
| * The generator intentionally uses Elem::vertex_average() rather than the true geometric centroid | |
| * to mirror the inexpensive point-sampling behavior of other subdomain tagging mesh generators in | |
| * MOOSE. |
| #include <unordered_map> | ||
| #include <vector> | ||
|
|
||
| class TriangleManifold |
| _point_locator(_mesh.sub_point_locator()) | ||
| { | ||
| mooseAssert(_surface_tolerance > 0.0, "surface_tolerance must be strictly positive."); | ||
|
|
| bool | ||
| TriangleManifold::pointOnSurface(const Point & point) const | ||
| { | ||
| return (*_point_locator)(point) != nullptr; |
There was a problem hiding this comment.
I dont understand that.
maybe pointInsideSurface?
the point locator would return an element not just for points on the surfacew
There was a problem hiding this comment.
This checks if the point is close to the surface, not inside or outside. So I think pointOnSurface is appropriate. The point locator would return a nullptr if the point lies far enough away from the surface, as defined by _point_locator->set_close_to_point_tol(_surface_tolerance);.
Implementation of a mesh generator that assigns a subdomain based on a STL file.
A new utilility class---
STLManifold---is implemented, which determines whether a point is enclosed in the STL geometry.A new mesh generator---
STLSubdomainGenerator---is implemented, which assigns a specified subdomain based on whether the element centroid is within the manifold specified by the STL file.Closes #32763